feat: migrate callers off FEATURES-as-dict access#38772
Draft
feanil wants to merge 30 commits into
Draft
Conversation
feanil
force-pushed
the
feanil/fix-deprecation-warnings
branch
from
1b8c4e8 to
7f5dbc2
Compare
feanil
force-pushed
the
feanil/features-dict-migration
branch
3 times, most recently
from
9ffdb02 to
f9359f9
Compare
feanil
force-pushed
the
feanil/features-dict-migration
branch
4 times, most recently
from
60dd592 to
4ecd09f
Compare
feanil
force-pushed
the
feanil/features-dict-migration
branch
4 times, most recently
from
858d762 to
346382f
Compare
This was referenced Jun 26, 2026
feanil
force-pushed
the
feanil/features-dict-migration
branch
from
346382f to
520006a
Compare
kdmccormick
approved these changes
Jul 8, 2026
kdmccormick
left a comment
kdmccormick
left a comment
Member
There was a problem hiding this comment.
looks good once rebased & green.
As I mentioned before I dislike all the new getattr invocations, but I've come around that it's the right way to push through this FEATURES refactor. I wrote up a followup ticket which we can put into the config simplification epic somewhere post-FEATURES-refactor #38861
Comment on lines
11
to
+12
| TPA_FEATURES_KEY = 'ENABLE_THIRD_PARTY_AUTH' | ||
| TPA_FEATURE_ENABLED = TPA_FEATURES_KEY in settings.FEATURES | ||
| TPA_FEATURE_ENABLED = hasattr(settings, TPA_FEATURES_KEY) |
Member
There was a problem hiding this comment.
not your problem to fix, but 🤮🤮🤮
feanil
force-pushed
the
feanil/features-dict-bridge
branch
from
3a686d6 to
1299ef1
Compare
feanil
force-pushed
the
feanil/features-dict-migration
branch
from
520006a to
20636c5
Compare
feanil
force-pushed
the
feanil/features-dict-migration
branch
4 times, most recently
from
df5644a to
7009266
Compare
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Production reads use getattr(settings, ...); tests use @override_settings(ENABLE_CREATOR_GROUP=...). Mixed-flag patches keep the other flag in patch.dict until its own migration. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- Production reads use getattr(settings, ...); tests use @override_settings(CERTIFICATES_HTML_VIEW=...). - Drop now-unused FEATURES_WITH_CERTS_ENABLED/_DISABLED helpers. - FEATURES_WITH_CUSTOM_CERTS_ENABLED keeps CUSTOM_CERTIFICATE_TEMPLATES_ENABLED until that flag migrates. - Mixed-flag patches keep the other flag in patch.dict. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
feanil
force-pushed
the
feanil/features-dict-migration
branch
from
c3b0a3b to
1515d53
Compare
- test_200_for_lti_provider's post-`with` modulestore assertions relied on a state leak (the old code aliased settings.FEATURES without .copy(), so the mutation persisted). They now run inside the with-block. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- Tests that previously had @patch.dict on setUp/setUpClass move the override to the class level. @override_settings as a setUp decorator conflicts with CacheIsolationMixin's own override_settings push/pop — the wrong layer gets popped on setUp exit, leaving settings.CACHES broken for subsequent test methods. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Running `pytest openedx/core/djangoapps/embargo/tests/` in isolation fails one test because `test_redirect_if_blocked_courseware` mocks `embargo.api.check_course_access`, and the mocked call triggers the first-time import of `embargo.views`. views.py's `from .api import check_course_access` captures the MagicMock as `views.check_course_access` permanently. CI doesn't hit this because earlier shard directories preload `embargo.views` before any mock fires. Reproduces on master. Calling via the module attribute resolves at call time, so future mocks on the api function stay visible to the view. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…-dict Default flips from raising KeyError (`settings.FEATURES['…']` direct subscript, only safe inside an active patch) to False (`getattr` with default) — the flag isn't defined in any env file. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…s-dict Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Flat setting was already being assigned to via the instructor plugin's plugin_settings hook (settings.MAX_ENROLLMENT_INSTR_BUTTONS = 200) — only the readers were still going through settings.FEATURES. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…as-dict Flat setting was already being assigned in the instructor plugin's plugin_settings hook — only the two readers were still going through settings.FEATURES.get. Also fixes the annotation that read FEATURES['ENABLE_CERTIFICATES_INSTRUCTOR_MANAGE] (missing closing quote, wrong location). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…dict Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
feanil
force-pushed
the
feanil/features-dict-migration
branch
from
113517b to
873ee59
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Second PR in the FEATURES-as-dict migration stack. #38818 (the bridge) is merged.
Per-flag
refactor:commits that move callers offsettings.FEATURES['X']/patch.dict(settings.FEATURES, …)and ontosettings.X/@override_settings(X=…). The underlying flat settings already exist inlms/envs/common.py,cms/envs/common.py, and the sharedopenedx/envs/common.py— the env files were previously flattened, so this PR only touches call sites (plus a few flag definitions that needed to move into the shared file).settings.Xvsgetattr(settings, 'X', <default>)Most reads drop the
getattrfallback in favor of a baresettings.X— but not all. The rule is: a baresettings.Xrequires that the setting be defined in thecommon.pyfile of every process that can reach the read.openedx/envs/common.py(shared, imported by both LMS and CMS) — use baresettings.Xat every caller.lms/envs/common.py— use baresettings.Xonly if the caller is exclusively reachable from the LMS process. If the caller lives undercommon/djangoapps/oropenedx/core/and can be pulled in by CMS test collection or CMS request handling, keepgetattr(settings, 'X', <default>)so CMS doesn'tAttributeError.cms/envs/common.py— the mirror rule. Baresettings.Xat CMS-only callers;getattrat shared/LMS-reachable callers. This deliberately keeps Studio-only concepts (e.g.ENABLE_CREATOR_GROUP,DISABLE_COURSE_CREATION) from leaking into LMS settings... toggle_name:/.. setting_name:annotation) in the smallest common file that covers all callers, then dropsgetattrat the call sites.hasattr(settings, 'X')is used in a few module-load contexts (test util constants) where we specifically want "is the setting defined" rather than "what is its value" — safe against a missing setting on either process.Stack: